-
Notifications
You must be signed in to change notification settings - Fork 295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Bump cluster-api dependency to v1.5.0-beta.1 #1970
✨ Bump cluster-api dependency to v1.5.0-beta.1 #1970
Conversation
/test pull-cluster-api-provider-vsphere-verify-crds |
216da21
to
bf94956
Compare
/retest |
Note: I'm not clear yet how flaky the existing tests are or if it is the PR. |
/test pull-cluster-api-provider-vsphere-integration-test |
/test pull-cluster-api-provider-vsphere-integration-test |
1c575d4
to
e8609c3
Compare
/test pull-cluster-api-provider-vsphere-verify-gen |
e8609c3
to
a67c467
Compare
/test help |
@chrischdi: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/test pull-cluster-api-provider-vsphere-full-e2e |
Failure of |
a67c467
to
1ccaabd
Compare
1ccaabd
to
05dc4f3
Compare
/test pull-cluster-api-provider-vsphere-full-e2e |
/test pull-cluster-api-provider-vsphere-verify-gen |
CI looks good.
|
05dc4f3
to
5ea79aa
Compare
/test pull-cluster-api-provider-vsphere-full-e2e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New to CAPV, but from a CAPI/CR perspective overall this looks good
go.mod
Outdated
@@ -1,18 +1,18 @@ | |||
module sigs.k8s.io/cluster-api-provider-vsphere | |||
|
|||
go 1.19 | |||
go 1.20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know enough about CAPV yet, but this usually requires bumping images used in Prow. Let's check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valid point. Checked in test-infra:
The used images provide go 1.19.9.
So I reverted the go.mod version change for now. Let's keep this separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you open a follow-up issue to track this, please?
main.go
Outdated
@@ -368,3 +372,22 @@ func runProfiler(addr string) { | |||
setupLog.Error(err, "problem running profiler server") | |||
} | |||
} | |||
|
|||
func isCRDLoaded(mgr ctrlmgr.Manager, gvr schema.GroupVersionResource) (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe isCRDDeployed would be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have e2e coverage that this new check works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is found very fast, because the controller won't get added and e2e tests would fail. (That's also how I recognized it).
Do you think that's already sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was mostly wondering if we have e2e tests for both cases (now especially supervisor :D).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be good. I guess we have at least test coverage for the first case and if it works there it should also work for supervisor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both cases are tested:
For supervisor: via make test-integration
.
For non-supervisor: via make e2e
.
Going to close out remaining dependabot PRs. Can we therefore get the following in?
|
bumped all three with the new commit. |
/test pull-cluster-api-provider-vsphere-verify-crds |
1 similar comment
/test pull-cluster-api-provider-vsphere-verify-crds |
Let's rebase again :P |
…/tools Compiling kustomize broke with bumping the go modules. Adopting the way upstream CAPI uses to build the kustomize binary instead.
Webhooks are now able to return warning errors.
Fake client requires to have a finalizer set when a DeletionTimestamp is set on object creation.
The returned errors of controller-runtime did change so the detection did not work anymore.
…client for objects we update .status
Beginning with Go 1.20 rand gets auto-seeded on startup.
…ts due to bind address already in use
468352d
to
7805709
Compare
@chrischdi: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: randomvariable The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
This patch bumps the CAPI module dependency to `v1.5.0-beta.0 to integrate the latest minor beta release of CAPI with the CAPV release.
I tried to make meaningful commits for better reviewability. Summary:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #1969
Special notes for your reviewer:
n/a
Release note: